Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add existence checking to indexed map #700

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

shanev
Copy link
Contributor

@shanev shanev commented Apr 12, 2022

Had the need for existence checking so added has() to indexed map. Hope I did it correctly :)

Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
Thank you for your contribution.

Just nitpicking - I see you've copied from other files (which is fine), though still part of test is rather redundant.
For example, there's no point in creating references in tests, since we don't care about performance there. To be extra picky, load it properly section is tested in other cases as well.

Whole test case could basically look like this:

    #[test]
    fn existence() {
        let mut store = MockStorage::new();
        let map = build_map();
        let (pks, _) = save_data(&mut store, &map);

        assert!(map.has(&store, pks[0]));
        assert!(!map.has(&store, "6"));
    }

packages/storage-plus/src/indexed_map.rs Outdated Show resolved Hide resolved
shanev and others added 2 commits April 14, 2022 09:26
Co-authored-by: Jakub Bogucki <software-solutions@tuta.io>
@shanev
Copy link
Contributor Author

shanev commented Apr 14, 2022

@ueco-jb Simplified the test. Thanks for the feedback.

@ueco-jb ueco-jb merged commit 44df93f into CosmWasm:main Apr 14, 2022
@shanev shanev deleted the shanev/indexedmap-has branch April 14, 2022 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants